Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiremote support for TestLibrary #489

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Siolto
Copy link
Collaborator

@Siolto Siolto commented Jun 20, 2023

Create multiple facades, depending on the amount of configured capabilities

Siolto added 2 commits June 20, 2023 13:38
- initialize seperate facades for every browserInstance we have

- add tests for the multiremote support
Copy link
Contributor

@vobu vobu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scope fn questions

@@ -92,8 +92,8 @@ function initBrowser(browserInstance: WebdriverIO.Browser) {

_addWdi5Commands(browserInstance)

if (!(browserInstance as any).fe) {
;(browserInstance as any).fe = WDI5FE
if (!(browser as any).fe) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, browserInstance is explicitly passed in here → why is it not supposed to be used? IMO the browserInstance helps retain scope within the fn by explicitly not referencing the global browser

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we would have to initialize the facade as often as we have defined browsers in the config. That would look like something like this:

fioriElementsFacadeOne = await one.fe.initialize{...}

fioriElementsFacadeTwo = await two.fe.initialize{...}

We can do that of course, I just don't now if this is the best way

await loadFELibraries(browserInstance)
await initOPA(appConfig, browserInstance)
return new WDI5FE(appConfig, browserInstance)
static async initialize(appConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar here - why remove browserInstance as parameter and instead break scope of the otherwise "pure" function?

Copy link
Collaborator Author

@Siolto Siolto Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we never passed the second parameter to the initialize function. We only passed the config and just took the default for browserInstance

We could stick to that pattern, but then we have to pass the relevant browserInstance for the multiRemote in the before hook:

before(async () => {
        FioriElementsFacade = await browser.fe.initialize({appconfig}, browserInstance)
})

I guess we just have to decide what is cleaner

@Siolto
Copy link
Collaborator Author

Siolto commented Jun 20, 2023

scope fn questions

valid questions. I guess we have to do a short sync to decide which way is cleaner.

@vobu
Copy link
Contributor

vobu commented Jul 14, 2023

after an ad-hoc maintainer meeting (😸), we agreed to

  • move the entire test library setup into the config (wdio*.conf.*), thus
  • taking out the need to .initialize({}) from the test-scope and move it into the bootstrap of wdi5
  • safe-guard the existing browser.fe.initialize(config) when called at test-scope so we don't cause side effects when moved into bootstrap-scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants